New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to experimental features #7242
Conversation
@@ -1758,8 +1758,10 @@ private List<CompletionResult> GetResultForAttributeArgument(CompletionContext c | |||
List<CompletionResult> result = new List<CompletionResult>(); | |||
foreach (PropertyInfo pro in propertyInfos) | |||
{ | |||
//Ignore TypeId (all attributes inherit it) | |||
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did the != "TypeId
go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property TypeId
is a read-only property. This change adds two more read-only properties to ParameterAttribute
: ExperimentName
and ExperimentAction
. They shouldn't show up in the tab completion of Attribute properties because you cannot set values for them. So we should just ignore all getter properties.
} | ||
private ExperimentAction _effectiveAction = default(ExperimentAction); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
else | ||
{ | ||
s_PSSnapInTracer.WriteLine("Executing IModuleAssemblyInitializer.Import for {0}", assemblyPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like the snapin related changes would be better in a different PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, good catch here!
I reviewed my changes here again and this if...else...
block shouldn't be removed. We should run ModuleInitializer
when importing a binary module, even if the module was already loaded through an earlier import. I will revert this change.
} | ||
|
||
Array.Sort(featureNames); | ||
string allNames = String.Join(Environment.NewLine, featureNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's better to join with a static value instead of NewLine? Is NewLine static or based on culture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think newline is platform based: \n
vs \r\n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into it, Environment.NewLine
is compile-time static (the way corefx does it is they have a partial class with the shared part and then a .Unix.cs
and a .Windows.cs
, etc. and then they conditionally include it in compilation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Environment.NewLine
is static, \r\n
on Windows and \n
on Unix platforms. Here we are calculating a hash that is used locally for the analysis cache file name, so it's OK as long as it's consistent on a specific platform.
@@ -81,6 +81,16 @@ | |||
InnerException: {3} | |||
</value> | |||
</data> | |||
<data name="PS_PROVIDEReventE_O_ExperimentalFeatureInvalidName" xml:space="preserve"> | |||
<value>Experimental Feature Initialization: Ignore the experimental feature '{0}' from the config file. {1}</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems on that the message doesn't say the name is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The {1}
part will say why this name is invalid.
/// <summary> | ||
/// Search module paths to find all available experimental features. | ||
/// </summary> | ||
[Parameter()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses are used here but not above in a no-arg attribute -- should probably be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@iSazonov I didn't make many style changes (maybe |
yield return new FormatViewDefinition("ExperimentalFeature", | ||
TableControl.Create() | ||
.AddHeader(Alignment.Left, width: 35) | ||
.AddHeader(Alignment.Right, width: 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the Enabled
column width 10? Why not 7 (length of Enabled
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Fixed.
{ | ||
yield return new FormatViewDefinition("ExperimentalFeature", | ||
TableControl.Create() | ||
.AddHeader(Alignment.Left, width: 35) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like Name
s should be terse (and unique), so 35 seems quite wide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When it's a module feature (feature declared in module), the feature name would be longer than the module name. The width for module name is 35, and I also use 35 here for the feature name. If you still think we should use a smaller size, I can make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, for a module it makes sense. Let's leave it at 35 for now. Formatting changes aren't considered breaking so we can adjust later.
TableControl.Create() | ||
.AddHeader(Alignment.Left, width: 35) | ||
.AddHeader(Alignment.Right, width: 10) | ||
.AddHeader(Alignment.Left, width: 35) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
35 here also seems wide for the Source
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a module feature, the source is the path to the module manifest file. That's why I also make the width 35. Let me know if you think we should use a smaller size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for now.
|
||
using System.Collections.Generic; | ||
using System.Collections.ObjectModel; | ||
using System.Collections.Immutable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I
comes before O
. You almost have these in alphabetical order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the fix.
int firstDotIndex = featureName.IndexOf('.'); | ||
int lastDotIndex = featureName.LastIndexOf('.'); | ||
|
||
bool legit = firstDotIndex > 0 && lastDotIndex < featureName.Length - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this more performant than just
bool legit = !featureName.StartsWith(".") && !featureName.EndsWith(".")
? Which seems a bit more descriptive.
object privateData = null; | ||
if (data.Contains("PrivateData")) | ||
// Set the private data member for the module if the manifest contains this member | ||
object privateData = data["PrivateData"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrivateDate
and PSData
should be members of SpecialVariables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But they are not variables ... They are module manifest keys.
We could have a static type called ModuleManifestKeys
, where we can define all manifest key const strings just like SpecialVariables
. But we can do it in a separate PR.
$AssemblyPath = Join-Path $TestModule "ExpTest.dll" | ||
if (-not (Test-Path $AssemblyPath)) { | ||
## When using $SourcePath directly, 'Add-Type' fails in AppVeyor CI runs with an 'access denied' error. | ||
## It turns out Pester doesn't handle an exception like this from 'BeforeAll'. It causes the Pester to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we open an issue in Pester repo or does one already exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't open an issue yet. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add link to bug in comment? Don't want future tests following this pattern if it gets fixed in Pester.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do after I open the issue.
param($Name, $CommandType) | ||
$command = Get-Command $Name | ||
$command.CommandType | Should -Be $CommandType | ||
## 11 common parameters + '-Name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future maintenance of this code, it would be better to have something like:
BeforeAll {
$commonParameters = @('a','b','c',...)
}
...
$command.Parameters.Count | Should -Be ($commonParameters + "-Name").Length
Apply in all cases below. I think it removes the need for the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have used [System.Management.Automation.Internal.CommonParameters].GetProperties().Length
to get the count of common parameters. I will make the change.
} | ||
} | ||
|
||
#endregion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing a test cases for:
[Experimental()]
- ExperimentAction.None
- ExperimentName starting and ending with period
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests added.
#region Experimental Feature Related Properties | ||
|
||
/// <summary> | ||
/// Name of the experimental feature this attribute is associated with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be: "Gets name of the experimental feature this attribute is associated with."
public string ExperimentName { get; } | ||
|
||
/// <summary> | ||
/// Action for engine to take when the experimental feature is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Gets action for engine to take when the experimental feature is enabled."
internal bool ToShow => EffectiveAction == ExperimentAction.Show; | ||
|
||
/// <summary> | ||
/// Effective action to take at run time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Gets effective action to take at run time.".
//Ignore TypeId (all attributes inherit it) | ||
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase))) | ||
// Ignore getter-only properties, including 'TypeId' (all attributes inherit it). | ||
if (!property.CanWrite) { continue; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems our convention is
if (!property.CanWrite) continue;
or
if (!property.CanWrite)
{
continue;
}
I'd prefer second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use if () { // simple statement }
all over the code base. But I don't think we should allow the first pattern in your comment. The if body should always be in { }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly agree that we should always use braces.
In a world where we no longer use teletypes (i.e. where screens are big and newlines are cheap), I tend to feel that we should always just hang the line so that as my eyes scan down a document at an approximate indentation level and always know what's going on. But, that's just a style preference, and inline-braces are still pretty good to me.
|
||
using System.Collections.Generic; | ||
using System.Collections.ObjectModel; | ||
using System.Collections.Immutable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the fix.
} | ||
|
||
// If all 'ParameterAttribute' declared for the parameter are hidden due to | ||
// an experimental feature, then the parameter should be ignored. | ||
if (hasParameterAttribute && !hasEnabledParamAttribute) { return null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use multiline formatting.
attributes.Reverse(); | ||
if (!hasParameterAttribute) | ||
{ | ||
attributes.Insert(0, new ParameterAttribute()); | ||
} | ||
|
||
var result = new RuntimeDefinedParameter(parameterAst.Name.VariablePath.UserPath, parameterAst.StaticType, | ||
new Collection<Attribute>(attributes.ToArray())); | ||
new Collection<Attribute>(attributes)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting - please place one parameter per line.
{ | ||
var attributeAst = ParamBlock.Attributes[index]; | ||
var expAttr = GetExpAttributeHelper(attributeAst); | ||
if (expAttr != null) { yield return expAttr; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use multiline formatting.
{ | ||
return Compiler.GetAttribute(potentialExpAttr) as ExperimentalAttribute; | ||
} | ||
catch (Exception) { /* catch all and assume it's not a declaration of ExperimentalAttribute */ } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we get style issues in CodeFactor for the formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update this one as it's not exactly an empty catch body.
@@ -117,6 +117,9 @@ | |||
<resheader name="writer"> | |||
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value> | |||
</resheader> | |||
<data name="EnabledExperimentalFeatures" xml:space="preserve"> | |||
<value>Variable to hold the enabled experimental feature names</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add final dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the existing examples. I don't have a strong opinion. It looks OK to me without the dot:
PS:8> $v.Description
Parent folder of the host application of the current runspace
} | ||
|
||
It "Argument validation for constructors of 'ExperimentalAttribute' and 'ParameterAttribute'" { | ||
{ [Experimental]::new("", "None") } | Should -Throw -ErrorId "PSArgumentNullException" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to have these as TestCases so that the first failure doesn't prevent the rest from running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update the test.
@daxian-dbw I moved your style comment in Issue #4708. |
@iSazonov Thanks for moving it to the right place. I will spend more time refining the rules for CodeFactor after finishing the first check-in of the experimental feature work. |
//Ignore TypeId (all attributes inherit it) | ||
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase))) | ||
// Ignore getter-only properties, including 'TypeId' (all attributes inherit it). | ||
if (!property.CanWrite) { continue; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly agree that we should always use braces.
In a world where we no longer use teletypes (i.e. where screens are big and newlines are cheap), I tend to feel that we should always just hang the line so that as my eyes scan down a document at an approximate indentation level and always know what's going on. But, that's just a style preference, and inline-braces are still pretty good to me.
s_cacheStoreLocation = | ||
Environment.GetEnvironmentVariable("PSModuleAnalysisCachePath") ?? | ||
// If user defines a custom cache path, then use that. | ||
string userDefinedCachePath = Environment.GetEnvironmentVariable("PSModuleAnalysisCachePath"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it does constitute a public API, so we ought to document it, although there's not really much need
/// Engine features come before module features. | ||
/// Within engine features and more features, features are ordered by name. | ||
/// </remarks> | ||
private static string GetSortingString(ExperimentalFeature feature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daxian-dbw This is the only thing remaining I can think of. May not be significant, but returning (int, string)
here might save some allocations
/// </remarks> | ||
private static string GetSortingString(ExperimentalFeature feature) | ||
{ | ||
if (ExperimentalFeature.EngineSource.Equals(feature.Source)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be case insensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was assuming people will use the const string field ExperimentalFeature.EngineSource
for registering an engine exp feature, but it's more reliable to make this comparison case insensitive.
@TravisEz13 is on vacation. I assigned this PR to @adityapatwardhan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick. Looks good otherwise.
/> | ||
<string | ||
id="PS_PROVIDER.event.E_O_ExperimentalFeatureReadConfigError.message" | ||
value="Experimental Feature Initialization: Failed to read the config file.%n Exception: %1 %n Message: %2 %n StackTrace: %3 %n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a space needed between file.
and %n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%n
is a new line, so no space is needed here. For Exception: %1 %n Message: %2 %n StackTrace: %3 %n
, the space character before %n
is not necessary, but it's been that way in other places of this manifest file, so I followed the existing pattern.
@daxian-dbw Congratulations on this great work! 👍 |
The PR resolves some style issues in engine code. (Moved from PR #7242. )
PR Summary
RFC: PowerShell/PowerShell-RFC#114
Goals:
powershell.config.json
Function
,Cmdlet
, parameters and parameter sets to be shown to the user or hiden from the user depending on whether the associated experimetnal feature is on or off.Get-ExperimentalFeature
No Goals:
Engine Experimental Feature
Engine experimental features are required to register to the
ExperimentalFeature
type, by adding anExperimentalFeature
instance toExperimentalFeature.EngineExperimentalFeatures
in the type initailizer.Module Experimental Feature
Module experimetnal features can declared by adding
ExperimentalFeatures = @(..)
to thePSData
section ofPrivateData
in the module manifest. Example:Enable Experimental Features
Experimental features are enabled by declaring them in the
powershell.config.json
file. The enabled experimetnal features are fixed after powershell starts, and cannot be changed at run time.Exposed Public APIs
System.Management.Automation.ExperimentalAttribute
. An attribute that can be applied to types or properties or fields. The attribute is used to support showing or hiding a command (Function, Cmdlet), or a command parameter.ParameterAttribute
:Parameter(string experimentalFeatureName, ExperimentAction)
. This constructor is used to support showing or hiding a parameter set.System.Management.Automation.ExperimentAction
. An enum with 3 members: "None", "Show", "Hide". This enum is used when creating an instance ofExperimentalAttribute
, orParameterAttribute
with the new constructor. It indicate the action to take for a command or parameter when the associated feature is on or off.System.Management.Automation.ExperimentalFeature
. It represents an experimental feature, and it also exposes a static methodbool HasEnabled(string featureName)
for users to check if a given experimental feature is on.New Type Accelerators
ExperimentAction
forSystem.Management.Automation.ExperimentAction
Experimental
forSystem.Management.Automation.ExperimentalAttribute
ExperimentalFeature
forSystem.Management.Automation.ExperimentalFeature
New Cmdlet
Add
Get-ExperimentalFeature [[-Name] <string[]>] [-ListAvailable] [<CommonParameters>]
to retrieve information about experimental features from engine or modules.Name
takes wildcards. When-ListAvailable
is not specified,Get-ExperimentalFeature
only returns the enabled experimental features (mimicGet-Module
). When-ListAvailable
is specified,Get-ExperimentalFeature
returns all available experimental features from engine and modules in module paths.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.[feature]
if the change is significant or affects feature tests